Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MAINT: revendor xsimd #76

Merged
merged 4 commits into from
May 27, 2023
Merged

MAINT: revendor xsimd #76

merged 4 commits into from
May 27, 2023

Conversation

h-vetinari
Copy link
Member

@h-vetinari h-vetinari commented May 8, 2023

The current pin was added in 3fe8468 at the time of pythran 0.11 and not touched since. Before that, we pinned even harder, c.f. that commit:

-    - xsimd 7.6.*
+    - xsimd >=8.0.5,<8.1

There are no further comments in #63, but I suspect I didn't want to unpin to rashly and break something.

Edit: the thrust of this PR changed based on the discussion below, to revendor xsimd for the time being, rather than have to deal with pinning (sometimes unreleased, patched) xsimd versions, which is the only thing that pythran is testing/supporting at the moment.

@conda-forge-webservices
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

@rgommers
Copy link

rgommers commented May 8, 2023

I guess part of the issue here is the unvendoring of xsimd. As a result, there is no metadata in pythran itself (because the correct working version is always present in third_party/ inside the pythran repo.

For consideration:

  • there's lots of issues where a particular pythran release requires a particular version of xsimd,
  • on top of that there is the long-standing bug in conda itself with non-Bash shells which means the unvendoring breaks hard on some shells (xref BLD: detect xsimd if it's installed and add to pythran dependency scipy/scipy#18439)
  • this means that it's pretty clear that this unvendoring is a bad idea, and currently unreliable for any package using Pythran. I think it should be reverted. There really are no downsides to vendoring a header-only library.

@serge-sans-paille
Copy link
Contributor

serge-sans-paille commented May 8, 2023 via email

@h-vetinari h-vetinari changed the title unpin xsimd version DISCUSS: revendor xsimd or unpin its version? May 9, 2023
@h-vetinari
Copy link
Member Author

@conda-forge/xsimd and @SylvainCorlay specifically:

@rgommers: this means that it's pretty clear that this unvendoring [of xsimd] is a bad idea, and currently unreliable for any package using Pythran. I think it should be reverted. There really are no downsides to vendoring a header-only library.

@serge-sans-paille: I tend to agree with Ralf here. As xsimd gets more and more stable, the
situation may evolve, but the only setup we currently test is the vendoring one.

This would revert your request from a quite a while ago: #14 / #15. Any opposition?

Taken for pythran itself, I think the arguments are convincing (not least the header-only aspect!), but I'm not sure how that would interact with other uses of xsimd. The alternative is effectively mirroring the exact state of the xsimd headers per pythran version, which would be a pain e.g. if patches get backported (like serge-sans-paille/pythran#2104 recently)

FYI @isuruf

@eli-schwartz
Copy link

Declaring the same dependency but as conda metadata instead of vendored files should fix incompatibilities in general.

on top of that there is the long-standing bug in conda itself with non-Bash shells which means the unvendoring breaks hard on some shells

This could be solved by making pythran refer to xsimd using a symlink.

@rgommers
Copy link

rgommers commented May 9, 2023

but I'm not sure how that would interact with other uses of xsimd.

There should not be a problem there I'd think, unless there's a bug report to point to (not impossible some global symbol leaks, but seems unlikely).

Declaring the same dependency but as conda metadata instead of vendored files should fix incompatibilities in general.

That'd be a problem, because that is a == dependency on xsimd, which is typically not what you want - if another package has an incompatible constraint, the packages will not be installable together.

This could be solved by making pythran refer to xsimd using a symlink.

Doesn't work on Windows, and would be another hack. It'd be better to spend the time fixing the problems conda seems to have with compiler activation in non-Bash shells than doing this kind of thing imho.

@h-vetinari h-vetinari changed the title DISCUSS: revendor xsimd or unpin its version? DISCUSS: revendor xsimd or update/unpin its version? May 9, 2023
@SylvainCorlay
Copy link
Member

In general, the policy for conda-forge is to not vendor as much as possible.

@h-vetinari
Copy link
Member Author

In general, the policy for conda-forge is to not vendor as much as possible.

Yes. However, in this case it's causing compatibility problems due to how tightly pythran and xsimd are connected, and it brings essentially no benefit because it's header only.

So we want to revendor despite conda-forge's policy, which I'm well aware of.

The question is whether that would negatively impact xsimd usecases somehow.

@eli-schwartz
Copy link

That'd be a problem, because that is a == dependency on xsimd, which is typically not what you want - if another package has an incompatible constraint, the packages will not be installable together.

It's not a problem that causes pythran or xsimd to have mysteriously missing headers. It's just the same problem that happens any time projects are insistent on what version of something they want. This isn't a problem unique to xsimd -- although you actually CAN solve it for C/C++ headers, since you can stuff them in versioned directories and have them be parallel installable, just like having both gcc-10 and gcc-13 installed.

Python software can't do that, of course.

Doesn't work on Windows, and would be another hack. It'd be better to spend the time fixing the problems conda seems to have with compiler activation in non-Bash shells than doing this kind of thing imho.

"solved on really recent versions of Windows", supposedly. 🤣

You can emulate it though -- adding a stub header into the pythran package that does #include "../../../path/to/global/xsimd/header.hpp" should be compatible with idealistic vendoring policies to the same extent that symlinks are compatible.

@eli-schwartz
Copy link

Yes. However, in this case it's causing compatibility problems due to how tightly pythran and xsimd are connected, and it brings essentially no benefit because it's header only.

At least one benefit it brings is the ability to generate an SBOM from the list of conda packages without looking into package-specific details of what is vendored.

The argument that something is special because header only, is a pretty terrible argument IMO. If everyone did this then disaster would quickly ensue and no one would be able to keep track of all their vendored code. There are more ideological reasons to argue against vendored code than just "you don't have to build it multiple times if you already built it once", and that's the only (non-)advantage that header-only libraries have (you have to build it the same number of times regardless of whether it is vendored or not).

The question is whether xsimd<->pythran are special compared to header-only libraries in general. Maybe they are, maybe they aren't, I don't know.

@h-vetinari
Copy link
Member Author

At least one benefit it brings is the ability to generate an SBOM from the list of conda packages without looking into package-specific details of what is vendored.

That assumes perfect unvendoring of everything in conda-forge, which despite our best efforts, is not the status quo. I don't think we advertise being able to generate SBOMs, and we shouldn't IMO, as that comes with a whole lot of rules that I don't think we're ready to fulfill.

The question is whether xsimd<->pythran are special compared to header-only libraries in general.

Using the wrong xsimd headers can break pythran, and as pythran's main developer said above: "the *only* setup we currently test is the vendoring one." And pythran doesn't even necessarily vendor exact versions, but may layer unreleased patches on top (and depend on those for correct operation). This is what happened around the release of 0.13 recently.

Unvendoring here comes with risks of breakage or untested behaviour changes, and for no real gain, as the headers need to get recompiled everywhere anyway, and we don't have an ABI of a compiled library that we have to keep stable across the ecosystem.

Of course I'm generally strongly in favour of unvendoring, but here it seems the net benefit is negative.

@eli-schwartz
Copy link

That assumes perfect unvendoring of everything in conda-forge, which despite our best efforts, is not the status quo. I don't think we advertise being able to generate SBOMs, and we shouldn't IMO, as that comes with a whole lot of rules that I don't think we're ready to fulfill.

I don't think that a failure to be perfect everywhere is an indictment on having a policy that it should be so. Surely the solution is to be perfect everywhere, right? So the best way to be perfect everywhere is to start by being perfect in one more place than you were before, not by reducing coverage for the policy compliance.

Being able to scrape that SBOM data from a list of packages is, regardless, very useful seed data for someone checking into their dependencies.

It's also useful outside of producing SBOMs for the same reason it's useful for producing SBOMs -- because people can look into what they are using, track down the sources of bugs, especially platform-specific bugs, etc.

And pythran doesn't even necessarily vendor exact versions, but may layer unreleased patches on top (and depend on those for correct operation). This is what happened around the release of 0.13 recently.

IMHO it would be nice in such a case for projects to follow "release early, release often", especially when they control both sides of the stack. Particularly for bug fixes.

@h-vetinari
Copy link
Member Author

IMHO it would be nice in such a case for projects to follow "release early, release often", especially when they control both sides of the stack. Particularly for bug fixes.

We (conda-forge) don't control how upstream pythran/xsimd do their releases. It's fine to identify ways that things can be improved (and try to convince upstream to adopt those), but right now, the job here is to not ship broken packages for the status quo, not some future ideal.

Surely the solution is to be perfect everywhere, right?

Not without a comprehensive plan of what we're trying to do, otherwise it's just aimless churn that doesn't actually achieve anything (like reliable SBOMs). To my knowledge, no-one is talking about generating SBOMs based on conda-forge metadata, so my point is: SBOMs are not in scope here, by a vast margin of unresolved issues, and so philosophical fidelity with that utopia is completely besides the much more pressing point of dealing with the issues in the here and now.

@rgommers
Copy link

The abstract discussion is perhaps distracting a little bit from the current issue, which is that the dependency info is wrong and that has given us concrete and annoying problems. There are only two options here:

  1. keep the unvendoring and add the correct metadata, i.e. a == pin that gets updated for each pythran release
  2. remove the unvendoring

That choice is up to the feedstock maintainers & conda-forge team in the end; (2) seems more pragmatic to me, but (1) would also work. Could we please come to a decision?

@h-vetinari
Copy link
Member Author

Seeing that pythran currently contains an unreleased xsimd version, I'm going with the revendoring. If pythran switches to a model that's more amenable to unvendoring (or people want to create separate, patched xsimd-releases just so we can pin them here), changing this back is just a PR away.

@h-vetinari h-vetinari changed the title DISCUSS: revendor xsimd or update/unpin its version? DISCUSS: revendor xsimd May 27, 2023
@h-vetinari h-vetinari changed the title DISCUSS: revendor xsimd MAINT: revendor xsimd May 27, 2023
@h-vetinari h-vetinari merged commit 858511f into conda-forge:main May 27, 2023
@h-vetinari h-vetinari deleted the xsimd branch May 27, 2023 12:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants